ci(slack): escape mrkdwn + fetch timeout (sync notifier with OGAD)#509
Merged
Conversation
Keeps the shared notifier identical across repos: escape & < > in the note body, AbortSignal.timeout(10000) on the POST. Matches off-grid-ai-desktop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughUpdates scripts/notify-slack-release.mjs to escape release notes and product text for Slack mrkdwn safety via a new esc() helper, adjusts message body fallback logic, and adds a 10-second AbortSignal timeout to Slack webhook and postMessage fetch calls. ChangesSlack release script safety fixes
Estimated code review effort: 1 (Trivial) | ~5 minutes ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
scripts/notify-slack-release.mjsParsing error: Unexpected token { Comment |
PR Summary by QodoCI: Escape Slack mrkdwn and add 10s timeout to release notifier
AI Description
Diagram
High-Level Assessment
Files changed (1)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
36 rules 1. Slack notifier missing regression test
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+38
to
54
| // Slack mrkdwn reserves & < > — a raw commit subject containing them (release notes are raw | ||
| // commit subjects) would misrender or be read as a <link|mention>. Escape the note body only; | ||
| // NOT the intentional <url|label> link line below. | ||
| const esc = (s) => s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | ||
|
|
||
| let notes = ''; | ||
| try { notes = readFileSync(notesFile, 'utf8').trim(); } catch { /* notes optional */ } | ||
| // Slack section text caps at 3000 chars; keep well under and never dump a wall. | ||
| if (notes.length > 2600) { notes = `${notes.slice(0, 2600)}\n…`; } | ||
|
|
||
| const tag = label ? ` \`${label}\`` : ''; | ||
| const header = `:package: *${product}* \`${version || 'release'}\`${tag}`; | ||
| const header = `:package: *${esc(product)}* \`${version || 'release'}\`${tag}`; | ||
| const linkLine = releaseUrl ? `<${releaseUrl}|Download / release page>` : ''; | ||
| const body = notes || '_No release notes generated for this build._'; | ||
| const body = notes ? esc(notes) : '_No release notes generated for this build._'; | ||
|
|
||
| const blocks = [ | ||
| { type: 'section', text: { type: 'mrkdwn', text: header } }, |
There was a problem hiding this comment.
1. Slack notifier missing regression test 📘 Rule violation ▣ Testability
The PR changes Slack message escaping and adds fetch timeouts, which alters release-notification behavior but does not include an automated regression test to pin the new behavior. This risks silent regressions in the release workflow and violates the requirement to add/update tests for behavior changes.
Agent Prompt
## Issue description
`scripts/notify-slack-release.mjs` behavior changed (mrkdwn escaping + request timeout), but there is no regression test added/updated in this PR to ensure the new behavior stays correct.
## Issue Context
This script runs in release workflows and is part of the release job’s observable behavior (Slack message formatting and network-call timeout). A regression test should verify escaping of `&`, `<`, `>` in the note/body/header and ensure fetch calls include a timeout signal.
## Fix Focus Areas
- scripts/notify-slack-release.mjs[38-78]
- __tests__/unit/scripts/notify-slack-release.test.ts[1-200]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+41
to
+51
| const esc = (s) => s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | ||
|
|
||
| let notes = ''; | ||
| try { notes = readFileSync(notesFile, 'utf8').trim(); } catch { /* notes optional */ } | ||
| // Slack section text caps at 3000 chars; keep well under and never dump a wall. | ||
| if (notes.length > 2600) { notes = `${notes.slice(0, 2600)}\n…`; } | ||
|
|
||
| const tag = label ? ` \`${label}\`` : ''; | ||
| const header = `:package: *${product}* \`${version || 'release'}\`${tag}`; | ||
| const header = `:package: *${esc(product)}* \`${version || 'release'}\`${tag}`; | ||
| const linkLine = releaseUrl ? `<${releaseUrl}|Download / release page>` : ''; | ||
| const body = notes || '_No release notes generated for this build._'; | ||
| const body = notes ? esc(notes) : '_No release notes generated for this build._'; |
There was a problem hiding this comment.
2. Escape breaks length cap 🐞 Bug ≡ Correctness
In notify-slack-release.mjs, release notes are truncated to 2600 characters before being escaped, but esc() can expand the text (e.g., '&' → '&') and push the final mrkdwn section over the 3000-character Slack limit. This can cause Slack to reject the block/payload and drop the release announcement despite the pre-escape truncation.
Agent Prompt
### Issue description
The script truncates `notes` to 2600 chars before escaping `&`, `<`, `>` via `esc()`. Because escaping expands characters into multi-byte entities, the final `body` text can exceed Slack’s 3000-char limit for section text, causing the announcement request to fail.
### Issue Context
This script is explicitly trying to stay under Slack’s 3000-character cap, but the cap is currently enforced on the *unescaped* text rather than the final rendered/posted mrkdwn string.
### Fix Focus Areas
- scripts/notify-slack-release.mjs[41-51]
### Recommended fix
Implement an `escapeAndTruncateMrkdwn(text, maxLen)` helper that:
1) Escapes `&`, `<`, `>` while building the output.
2) Stops when the *escaped output* reaches `maxLen` (e.g. 2600).
3) Appends `\n…` when truncation occurs.
This avoids post-escape length expansion and avoids slicing in the middle of an escape sequence (e.g., cutting `&` into `&am`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+38
to
+49
| // Slack mrkdwn reserves & < > — a raw commit subject containing them (release notes are raw | ||
| // commit subjects) would misrender or be read as a <link|mention>. Escape the note body only; | ||
| // NOT the intentional <url|label> link line below. | ||
| const esc = (s) => s.replace(/&/g, '&').replace(/</g, '<').replace(/>/g, '>'); | ||
|
|
||
| let notes = ''; | ||
| try { notes = readFileSync(notesFile, 'utf8').trim(); } catch { /* notes optional */ } | ||
| // Slack section text caps at 3000 chars; keep well under and never dump a wall. | ||
| if (notes.length > 2600) { notes = `${notes.slice(0, 2600)}\n…`; } | ||
|
|
||
| const tag = label ? ` \`${label}\`` : ''; | ||
| const header = `:package: *${product}* \`${version || 'release'}\`${tag}`; | ||
| const header = `:package: *${esc(product)}* \`${version || 'release'}\`${tag}`; |
There was a problem hiding this comment.
3. Escape comment mismatch 🐞 Bug ⚙ Maintainability
The new comment says only the note body is escaped, but esc() is also applied to the header product string. This mismatch is misleading for future maintainers reading the comment to understand what is (and isn’t) escaped.
Agent Prompt
### Issue description
A comment claims the script escapes "the note body only", but the code also escapes `product` in the header. This is a documentation/maintainability issue that can confuse future edits.
### Issue Context
Escaping is correctly *not* applied to the explicit Slack link markup line (`<url|label>`). The comment should reflect that the non-link text portions (header/body) may be escaped.
### Fix Focus Areas
- scripts/notify-slack-release.mjs[38-49]
### Recommended fix
Update the comment to reflect the actual intent, e.g.:
- “Escape Slack-reserved `& < >` in user-controlled text (header + notes). Do not escape the explicit `<url|label>` link markup line below.”
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Syncs
scripts/notify-slack-release.mjswith the OGAD copy: escape& < >in the note body (raw commit subjects can misrender / be read as a Slack link) +AbortSignal.timeout(10000)so a hung Slack endpoint can't stall the release job. Fail-soft unchanged. Keeps the two repos' notifiers identical.🤖 Generated with Claude Code
Summary by CodeRabbit